Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
1037823 to
a5ecaf2
Compare
0c6dd47 to
a8c8f92
Compare
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds support for selecting the next part by PartId or PartInstanceId across API, job-worker and UI; refactors lookahead to use structured PartInstanceAndPieceInstancesInfos with nextTimeOffset support; adds lookahead offset utilities; reorganizes and expands lookahead tests. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User/UI
participant API as User Action API
participant JobWorker as Job Worker
participant Store as Part/Instance Store
participant Playout as PlayoutModel
UI->>API: setNext(playlistId, partOrInstanceId, isInstance)
API->>JobWorker: handleSetNextPart(payload)
alt isInstance == true
JobWorker->>Store: fetch PartInstance(nextPartInstanceId)
Store-->>JobWorker: PartInstance
JobWorker->>Playout: setNextPartFromPart(DBPartInstance)
else
JobWorker->>Store: fetch DBPart(nextPartId)
Store-->>JobWorker: DBPart
JobWorker->>Playout: setNextPartFromPart(DBPart)
end
Playout->>Playout: update next state (validate by id)
Playout->>JobWorker: trigger timeline update
JobWorker-->>API: done
API-->>UI: confirmation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
FYI: we (NRK) are very interested in this, as this is something our users have been asking for ages. |
|
The PR now includes the ability to also set the take point for orphaned partInstances that are already nexted. Tests may need to be updated, written for this, please test this as well. :) |
a46f9cb to
1640536
Compare
|
@nytamin Could you please take another look at the code? I've added a lot of new changes since the first review. I've also updated the PR description to reflect those changes. |
36a1b4c to
028e64c
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@packages/corelib/src/worker/studio.ts`:
- Around line 257-258: SetNextPartProps currently allows both nextPartId and
nextPartInstanceId to be omitted or both set; change it to a union that enforces
exactly one identifier is present by replacing the optional-pair with a
discriminated XOR: one branch with nextPartId: PartId and nextPartInstanceId?:
never, and the other with nextPartInstanceId: PartInstanceId and nextPartId?:
never; update any callers and add a small type guard function (e.g.,
isNextPartId(props): props is { nextPartId: PartId; nextPartInstanceId?: never }
) to help narrow the union where needed.
In
`@packages/job-worker/src/playout/lookahead/__tests__/lookaheadOffset/lookaheadOffset.test.ts`:
- Around line 260-263: The test asserts three result items
(expect(res).toHaveLength(3)) but only provides two expected lookaheadOffset
values; update the lookaheadOffset assertion to include the third expected value
so the arrays match. In the test file adjust the expect(res.map((o) =>
o.lookaheadOffset).sort()).toEqual([...]) to include the missing offset (so it
has three entries) to align with the three layer names (layer1_lookahead,
layer2_lookahead, layer3_lookahead) and the res variable.
- Around line 227-230: The test asserts res has 3 items but then compares
lookaheadOffset against only two values; update the offset assertion to ignore
undefined offsets: compute offsets = res.map(o => o.lookaheadOffset).filter(o =>
o !== undefined).sort((a,b) => a - b) and assert expect(offsets).toEqual([500,
1000]), leaving expect(res).toHaveLength(3) as-is (alternatively, if you
intended only two items, change the length assertion to 2).
In `@packages/job-worker/src/playout/lookahead/__tests__/utils.ts`:
- Around line 44-74: The test helper expectPartToMatch treats a nextTimeOffset
of 0 as falsy and skips the assertion branch; update the conditional to use a
nullish check (e.g., if (nextTimeOffset != null) or if (nextTimeOffset !==
undefined && nextTimeOffset !== null)) so that a legitimate 0 value triggers the
toHaveBeenNthCalledWith call that includes nextTimeOffset when invoking
findLookaheadObjectsForPartMock in expectPartToMatch.
In `@packages/job-worker/src/playout/lookahead/findForLayer.ts`:
- Around line 116-119: The usesInTransition boolean is computed with a truthy
check that treats 0 as false; update the logic in the PartAndPieces construction
(partInfo) to explicitly detect a defined numeric inTransitionStart (e.g. check
partInstanceInfo.calculatedTimings?.inTransitionStart !== undefined &&
partInstanceInfo.calculatedTimings?.inTransitionStart !== null or typeof ===
'number') so that 0 is accepted as a valid start and usesInTransition is true
when an offset exists.
- Around line 86-99: The code currently uses a truthy check on nextTimeOffset
which ignores 0; change the condition in the block that calls
findLookaheadObjectsForPart to explicitly test for null/undefined (e.g.
nextTimeOffset != null) so a zero offset is honored: use (nextTimeOffset != null
&& !partInstancesInfo.next) to decide when to call findLookaheadObjectsForPart
with nextTimeOffset, otherwise call the variant without the offset; update the
conditional that builds objs (and keep the same
arguments/currentPartId/layer/previousPart/partInfo) so zero offsets are applied
correctly.
In `@packages/job-worker/src/playout/lookahead/findObjects.ts`:
- Around line 146-148: The current lookup in allObjs.map uses
unprotectString(piece._id) === obj.pieceInstanceId which can miss temporary
pieces whose instance id is derived (e.g., via startPartId); update the lookup
inside allObjs.map to compare the object's pieceInstanceId against
getBestPieceInstanceId(piece) instead (i.e., use partInfo.pieces.find(piece =>
getBestPieceInstanceId(piece) === obj.pieceInstanceId)), keeping the rest of the
mapping logic the same so temporary pieces are not silently skipped; reference
functions/values: allObjs.map, partInfo.pieces, unprotectString,
obj.pieceInstanceId, and getBestPieceInstanceId.
In `@packages/job-worker/src/playout/lookahead/lookaheadOffset.ts`:
- Around line 197-201: The reduce is comparing raw a.piece.enable.start instead
of the numeric start computed by getStartValueFromEnable, which can be
string/undefined; update the logic that builds or filters beforeOrAt so each
entry carries the numeric start value (call getStartValueFromEnable for each
piece) and then change the reduce in the block that picks the best (the reduce
on beforeOrAt) to compare those numeric start values (e.g. compare a.startValue
> b.startValue) rather than a.piece.enable.start; ensure getStartValueFromEnable
is referenced (it's module-level) and used consistently when constructing
beforeOrAt so the comparison is type-safe and handles "now"/while-based enables.
In `@packages/shared-lib/package.json`:
- Line 43: package.json now pins "timeline-state-resolver-types" to a nightly
version but yarn.lock is missing that entry; run yarn install in the repository
root to update yarn.lock so the nightly dependency is resolved and then commit
the updated yarn.lock; verify the new entry for timeline-state-resolver-types
appears in yarn.lock and that CI passes.
🧹 Nitpick comments (9)
packages/job-worker/src/playout/model/implementation/PlayoutModelImpl.ts (1)
783-784: Redundant ID comparison (never fails after map lookup).The map lookup already keys by
_id, so the ID comparison is effectively a no-op and the comment is now misleading. Either drop the check or switch back to a reference check if identity is still required.♻️ Suggested cleanup
- // Make sure we were given the exact same object - if (storedPartInstance.partInstance._id !== partInstance.partInstance._id) - throw new Error(`PartInstance being set as next is not current`)packages/job-worker/src/playout/lookahead/index.ts (1)
39-42: Guard exportedfindLargestLookaheadDistancefor empty input.Now that this helper is exported, an empty array could yield
-Infinity. Consider a safe default.🛡️ Safer default
export function findLargestLookaheadDistance(mappings: Array<[string, MappingExt]>): number { + if (mappings.length === 0) return LOOKAHEAD_DEFAULT_SEARCH_DISTANCE const values = mappings.map(([_id, m]) => parseSearchDistance(m.lookaheadMaxSearchDistance)) return _.max(values) }packages/job-worker/src/playout/setNext.ts (1)
595-625: Type discrimination logic is inverted from typical convention.The check
!('part' in nextPartOrInstance)discriminates DBPart (which lacks a nestedpartproperty) from DBPartInstance (which has one). While functionally correct, this inverted check may confuse future maintainers. Consider adding a clarifying comment or using a type guard function for clarity.Additionally, the
awaitinside thePlayoutPartInstanceModelImplconstructor call (line 616-618) could benefit from extracting the fetch to a separate statement for better readability and error traceability.♻️ Suggested refactor for clarity
if (!('part' in nextPartOrInstance)) { + // nextPartOrInstance is a DBPart (no nested 'part' property) consumesQueuedSegmentId = doesPartConsumeQueuedSegmentId(playoutModel, nextPartOrInstance) await setNextPart( context, playoutModel, { part: nextPartOrInstance, consumesQueuedSegmentId, }, setManually, nextTimeOffset ) } else { + // nextPartOrInstance is a DBPartInstance (has nested 'part' property) + const partInstance = nextPartOrInstance as DBPartInstance + const pieceInstances = await context.directCollections.PieceInstances.findFetch({ + partInstanceId: partInstance._id, + }) await setNextPart( context, playoutModel, new PlayoutPartInstanceModelImpl( - nextPartOrInstance as DBPartInstance, - await context.directCollections.PieceInstances.findFetch({ - partInstanceId: nextPartOrInstance._id, - }), + partInstance, + pieceInstances, false, new QuickLoopService(context, playoutModel) ), setManually, nextTimeOffset ) }packages/job-worker/src/playout/setNextJobs.ts (1)
70-80: Consider guarding the timeline update.
updateTimelineis called unconditionally at line 80, even whennextPartOrInstanceis undefined (meaning no next part was actually set). While this may be harmless, it could be more efficient to skip the timeline update if no change occurred.♻️ Optional: guard timeline update
if (nextPartOrInstance) { await setNextPartFromPart( context, playoutModel, nextPartOrInstance, data.setManually ?? false, data.nextTimeOffset ) + await updateTimeline(context, playoutModel) } - - await updateTimeline(context, playoutModel) }packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx (1)
288-308: Complex disabled-state logic could benefit from clearer naming.The
getIsPlayFromHereDisabledmethod has intricate boolean logic that's difficult to follow. Consider:
- The variable
isSelectedTimeWithinBounds(line 292-296) actually checks if offset is beyond bounds (duration < offset), so the name is misleading.- The compound condition at line 298
(!take || !!partInstance?.orphaned)is hard to parse.♻️ Suggested refactor for readability
private getIsPlayFromHereDisabled(take: boolean = false): boolean { const offset = this.getTimePosition() ?? 0 const playlist = this.props.playlist const partInstance = this.getPartFromContext()?.instance - const isSelectedTimeWithinBounds = + const isOffsetBeyondExpectedDuration = (partInstance?.part.expectedDuration ?? partInstance?.part.displayDuration ?? partInstance?.part.expectedDurationWithTransition ?? 0) < offset - if (playlist && playlist?.activationId && (!take || !!partInstance?.orphaned)) { + const isPlaylistActive = playlist?.activationId + const isOrphanedInstance = !!partInstance?.orphaned + const isCurrentPartInstance = partInstance?._id === playlist?.currentPartInfo?.partInstanceId + + if (isPlaylistActive && (!take || isOrphanedInstance)) { if (!partInstance) return true - else { - return ( - (isSelectedTimeWithinBounds && partInstance._id === playlist.currentPartInfo?.partInstanceId) || - (!!partInstance.orphaned && partInstance._id === playlist.currentPartInfo?.partInstanceId) - ) - } + return (isOffsetBeyondExpectedDuration && isCurrentPartInstance) || + (isOrphanedInstance && isCurrentPartInstance) } return false }packages/job-worker/src/playout/lookahead/__tests__/findForLayer/searchDistance.test.ts (1)
43-57: Test name could be more descriptive.The test "returns nothing when maxSearchDistance is too small" actually tests the case when
partInstancesInfois empty ({}), not specifically about search distance being too small. The mock setup with multiple return values (lines 44-50) is never used since no calls occur.Consider renaming to clarify the actual scenario being tested.
♻️ Suggested rename
- test('returns nothing when maxSearchDistance is too small', () => { + test('returns nothing when no partInstances are provided', () => { findLookaheadObjectsForPartMock - .mockReturnValue([]) - .mockReturnValueOnce(['t0', 't1'] as any) - .mockReturnValueOnce(['t2', 't3'] as any) - .mockReturnValueOnce(['t4', 't5'] as any) - .mockReturnValueOnce(['t6', 't7'] as any) - .mockReturnValueOnce(['t8', 't9'] as any) + .mockReturnValue([]) const res = findLookaheadForLayer(context, {}, orderedParts, layer, 1, 1, null)packages/job-worker/src/playout/lookahead/__tests__/findForLayer/basicBehavior.test.ts (1)
26-33: Consider asserting the result in addition to mock calls.The test verifies that
findLookaheadObjectsForPartMockis called withundefinedfor the previous part, but doesn't assert the actual return value fromfindLookaheadForLayer. Adding result assertions would strengthen the test.♻️ Suggested enhancement
test('if the previous part is unset', () => { findLookaheadObjectsForPartMock.mockReturnValue([]) - findLookaheadForLayer(context, { previous: undefined, current, next: nextFuture }, [], layer, 1, 1) + const res = findLookaheadForLayer(context, { previous: undefined, current, next: nextFuture }, [], layer, 1, 1) + expect(res.timed).toHaveLength(0) + expect(res.future).toHaveLength(0) expect(findLookaheadObjectsForPartMock).toHaveBeenCalledTimes(2) expectInstancesToMatch(findLookaheadObjectsForPartMock, 1, layer, current, undefined) })packages/job-worker/src/playout/lookahead/__tests__/findForLayer/orderedParts.test.ts (1)
11-25: RedundantbeforeEachblock.There are two
beforeEachblocks that both reset the same mock (lines 13-15 and lines 21-23). The outer one is sufficient; the inner one can be removed.♻️ Suggested cleanup
beforeEach(() => { findLookaheadObjectsForPartMock.mockReset() }) const orderedParts = findForLayerTestConstants.orderedParts const layer = findForLayerTestConstants.layer describe('findLookaheadForLayer - orderedParts', () => { - beforeEach(() => { - findLookaheadObjectsForPartMock.mockReset() - }) - const context = setupDefaultJobEnvironment()packages/job-worker/src/playout/lookahead/__tests__/findForLayer/constants.ts (1)
5-5: Random layer string may complicate test debugging.Using
getRandomString()for the layer means each test run uses a different value. While this provides isolation, it can make debugging failing tests harder since you can't easily reproduce with the same layer value. Consider using a fixed test layer string instead.♻️ Suggested change
-const layer: string = getRandomString() +const layer: string = 'test_layer'
| nextPartId?: PartId | ||
| nextPartInstanceId?: PartInstanceId |
There was a problem hiding this comment.
Enforce exactly one next identifier in SetNextPartProps.
Both fields being optional lets callers omit both or set both, which can silently produce invalid jobs. Suggest a union that requires exactly one.
✅ Proposed type guard
-export interface SetNextPartProps extends RundownPlayoutPropsBase {
- nextPartId?: PartId
- nextPartInstanceId?: PartInstanceId
- setManually?: boolean
- nextTimeOffset?: number
-}
+export type SetNextPartProps = RundownPlayoutPropsBase &
+ (
+ | { nextPartId: PartId; nextPartInstanceId?: never }
+ | { nextPartInstanceId: PartInstanceId; nextPartId?: never }
+ ) & {
+ setManually?: boolean
+ nextTimeOffset?: number
+ }🤖 Prompt for AI Agents
In `@packages/corelib/src/worker/studio.ts` around lines 257 - 258,
SetNextPartProps currently allows both nextPartId and nextPartInstanceId to be
omitted or both set; change it to a union that enforces exactly one identifier
is present by replacing the optional-pair with a discriminated XOR: one branch
with nextPartId: PartId and nextPartInstanceId?: never, and the other with
nextPartInstanceId: PartInstanceId and nextPartId?: never; update any callers
and add a small type guard function (e.g., isNextPartId(props): props is {
nextPartId: PartId; nextPartInstanceId?: never } ) to help narrow the union
where needed.
| expect(res).toHaveLength(3) | ||
| expect(res.map((o) => o.layer).sort()).toEqual([`layer1_lookahead`, 'layer2_lookahead', 'layer3_lookahead']) | ||
| expect(res.map((o) => o.lookaheadOffset).sort()).toEqual([1000, 500]) | ||
| }) |
There was a problem hiding this comment.
Assertion length mismatch.
The test expects res to have 3 elements (line 227), but the assertion on line 229 expects only 2 values [1000, 500]. If a third object has lookaheadOffset as undefined, the sorted array would be [1000, 500, undefined] (length 3), causing the assertion to fail. Either the expected length or the offset assertion needs adjustment.
🐛 Possible fix
expect(res).toHaveLength(3)
expect(res.map((o) => o.layer).sort()).toEqual([`layer1_lookahead`, 'layer2_lookahead', 'layer3_lookahead'])
- expect(res.map((o) => o.lookaheadOffset).sort()).toEqual([1000, 500])
+ expect(res.map((o) => o.lookaheadOffset).sort()).toEqual([500, 1000, undefined])Alternatively, if the intent is to only check defined offsets:
- expect(res.map((o) => o.lookaheadOffset).sort()).toEqual([1000, 500])
+ expect(res.map((o) => o.lookaheadOffset).filter((o) => o !== undefined).sort((a, b) => a - b)).toEqual([500, 1000])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(res).toHaveLength(3) | |
| expect(res.map((o) => o.layer).sort()).toEqual([`layer1_lookahead`, 'layer2_lookahead', 'layer3_lookahead']) | |
| expect(res.map((o) => o.lookaheadOffset).sort()).toEqual([1000, 500]) | |
| }) | |
| expect(res).toHaveLength(3) | |
| expect(res.map((o) => o.layer).sort()).toEqual([`layer1_lookahead`, 'layer2_lookahead', 'layer3_lookahead']) | |
| expect(res.map((o) => o.lookaheadOffset).filter((o) => o !== undefined).sort((a, b) => a - b)).toEqual([500, 1000]) | |
| }) |
🤖 Prompt for AI Agents
In
`@packages/job-worker/src/playout/lookahead/__tests__/lookaheadOffset/lookaheadOffset.test.ts`
around lines 227 - 230, The test asserts res has 3 items but then compares
lookaheadOffset against only two values; update the offset assertion to ignore
undefined offsets: compute offsets = res.map(o => o.lookaheadOffset).filter(o =>
o !== undefined).sort((a,b) => a - b) and assert expect(offsets).toEqual([500,
1000]), leaving expect(res).toHaveLength(3) as-is (alternatively, if you
intended only two items, change the length assertion to 2).
| expect(res).toHaveLength(3) | ||
| expect(res.map((o) => o.layer).sort()).toEqual([`layer1_lookahead`, 'layer2_lookahead', 'layer3_lookahead']) | ||
| expect(res.map((o) => o.lookaheadOffset).sort()).toEqual([1000, 500]) | ||
| }) |
There was a problem hiding this comment.
Same assertion length mismatch as above.
Line 260 expects 3 elements, but line 262 asserts only 2 offset values. This will likely fail for the same reason as the multi-layer test above.
🐛 Possible fix
expect(res).toHaveLength(3)
expect(res.map((o) => o.layer).sort()).toEqual([`layer1_lookahead`, 'layer2_lookahead', 'layer3_lookahead'])
- expect(res.map((o) => o.lookaheadOffset).sort()).toEqual([1000, 500])
+ expect(res.map((o) => o.lookaheadOffset).sort()).toEqual([500, 1000, undefined])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(res).toHaveLength(3) | |
| expect(res.map((o) => o.layer).sort()).toEqual([`layer1_lookahead`, 'layer2_lookahead', 'layer3_lookahead']) | |
| expect(res.map((o) => o.lookaheadOffset).sort()).toEqual([1000, 500]) | |
| }) | |
| expect(res).toHaveLength(3) | |
| expect(res.map((o) => o.layer).sort()).toEqual([`layer1_lookahead`, 'layer2_lookahead', 'layer3_lookahead']) | |
| expect(res.map((o) => o.lookaheadOffset).sort()).toEqual([500, 1000, undefined]) | |
| }) |
🤖 Prompt for AI Agents
In
`@packages/job-worker/src/playout/lookahead/__tests__/lookaheadOffset/lookaheadOffset.test.ts`
around lines 260 - 263, The test asserts three result items
(expect(res).toHaveLength(3)) but only provides two expected lookaheadOffset
values; update the lookaheadOffset assertion to include the third expected value
so the arrays match. In the test file adjust the expect(res.map((o) =>
o.lookaheadOffset).sort()).toEqual([...]) to include the missing offset (so it
has three entries) to align with the three layer names (layer1_lookahead,
layer2_lookahead, layer3_lookahead) and the res variable.
| export function expectPartToMatch( | ||
| findLookaheadObjectsForPartMock: TfindLookaheadObjectsForPart, | ||
| index: number, | ||
| layer: string, | ||
| partInfo: PartAndPieces, | ||
| previousPart: ReadonlyDeep<DBPart> | undefined, | ||
| currentPartInstanceId: PartInstanceId | null = null, | ||
| nextTimeOffset?: number | ||
| ): void { | ||
| if (nextTimeOffset) | ||
| expect(findLookaheadObjectsForPartMock).toHaveBeenNthCalledWith( | ||
| index, | ||
| context, | ||
| currentPartInstanceId, | ||
| layer, | ||
| previousPart, | ||
| partInfo, | ||
| null, | ||
| nextTimeOffset | ||
| ) | ||
| else | ||
| expect(findLookaheadObjectsForPartMock).toHaveBeenNthCalledWith( | ||
| index, | ||
| context, | ||
| currentPartInstanceId, | ||
| layer, | ||
| previousPart, | ||
| partInfo, | ||
| null | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "utils.ts" -path "*playout/lookahead/__tests__*"Repository: Sofie-Automation/sofie-core
Length of output: 133
🏁 Script executed:
cat -n ./packages/job-worker/src/playout/lookahead/__tests__/utils.ts | sed -n '44,74p'Repository: Sofie-Automation/sofie-core
Length of output: 964
🏁 Script executed:
rg "expectPartToMatch" ./packages/job-worker/src/playout/lookahead/__tests__/ -A 8 -B 2Repository: Sofie-Automation/sofie-core
Length of output: 4954
🏁 Script executed:
rg "expectPartToMatch" ./packages/job-worker/src/playout/lookahead/__tests__/ -A 10 | grep -E "(expectPartToMatch|nextTimeOffset|[0-9]+\))" | head -50Repository: Sofie-Automation/sofie-core
Length of output: 1620
🏁 Script executed:
rg "expectPartToMatch\(" ./packages/job-worker/src/playout/lookahead/__tests__/ -A 10 -B 2Repository: Sofie-Automation/sofie-core
Length of output: 3912
🏁 Script executed:
rg "expectPartToMatch.*,\s*\d+\s*\)" ./packages/job-worker/src/playout/lookahead/__tests__/ -B 2 -A 2Repository: Sofie-Automation/sofie-core
Length of output: 53
🏁 Script executed:
find ./packages/job-worker/src/playout/lookahead/__tests__/ -name "*.test.ts" -type f -exec grep -l "expectPartToMatch" {} \;Repository: Sofie-Automation/sofie-core
Length of output: 158
🏁 Script executed:
cat ./packages/job-worker/src/playout/lookahead/__tests__/findForLayer/orderedParts.test.ts | grep -A 8 "expectPartToMatch"Repository: Sofie-Automation/sofie-core
Length of output: 1182
🏁 Script executed:
grep -rn "expectPartToMatch(" ./packages/job-worker/src/playout/lookahead/__tests__/ --include="*.ts" | grep -v "export function"Repository: Sofie-Automation/sofie-core
Length of output: 820
🏁 Script executed:
sed -n '40,45p' ./packages/job-worker/src/playout/lookahead/__tests__/findForLayer/orderedParts.test.ts
sed -n '70,76p' ./packages/job-worker/src/playout/lookahead/__tests__/findForLayer/orderedParts.test.tsRepository: Sofie-Automation/sofie-core
Length of output: 660
🏁 Script executed:
find ./packages/job-worker/src/playout/lookahead/__tests__/ -name "*.test.ts" -type fRepository: Sofie-Automation/sofie-core
Length of output: 726
🏁 Script executed:
grep -rn "expectPartToMatch(" ./packages/job-worker/src/playout/lookahead/__tests__/ --include="*.test.ts"Repository: Sofie-Automation/sofie-core
Length of output: 820
Treat 0 as a valid nextTimeOffset in assertions.
if (nextTimeOffset) skips zero, so tests won't verify a legitimate 0 offset. Use a nullish check instead.
Suggested fix
- if (nextTimeOffset)
+ if (nextTimeOffset !== undefined && nextTimeOffset !== null)
expect(findLookaheadObjectsForPartMock).toHaveBeenNthCalledWith(
index,
context,
currentPartInstanceId,
layer,
previousPart,
partInfo,
null,
nextTimeOffset
)🤖 Prompt for AI Agents
In `@packages/job-worker/src/playout/lookahead/__tests__/utils.ts` around lines 44
- 74, The test helper expectPartToMatch treats a nextTimeOffset of 0 as falsy
and skips the assertion branch; update the conditional to use a nullish check
(e.g., if (nextTimeOffset != null) or if (nextTimeOffset !== undefined &&
nextTimeOffset !== null)) so that a legitimate 0 value triggers the
toHaveBeenNthCalledWith call that includes nextTimeOffset when invoking
findLookaheadObjectsForPartMock in expectPartToMatch.
| if (partInfo.pieces.length > 0 && isPartPlayable(partInfo.part)) { | ||
| const objs = findLookaheadObjectsForPart( | ||
| context, | ||
| currentPartInstanceId, | ||
| layer, | ||
| previousPart, | ||
| partInfo, | ||
| null | ||
| ) | ||
| const objs = | ||
| nextTimeOffset && !partInstancesInfo.next // apply the lookahead offset to the first future if an offset is set. | ||
| ? findLookaheadObjectsForPart( | ||
| context, | ||
| currentPartId, | ||
| layer, | ||
| previousPart, | ||
| partInfo, | ||
| null, | ||
| nextTimeOffset | ||
| ) | ||
| : findLookaheadObjectsForPart(context, currentPartId, layer, previousPart, partInfo, null) | ||
| res.future.push(...objs) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/job-worker/src/playout/lookahead/findForLayer.ts | head -150Repository: Sofie-Automation/sofie-core
Length of output: 5483
🏁 Script executed:
ast-grep --pattern 'function $_($$$nextTimeOffset$$$) {
$$$
}'Repository: Sofie-Automation/sofie-core
Length of output: 53
🏁 Script executed:
rg 'nextTimeOffset' packages/job-worker/src/playout/lookahead/ -B 3 -A 3Repository: Sofie-Automation/sofie-core
Length of output: 33607
Handle nextTimeOffset=0 explicitly. Truthy checks skip zero, so an explicit 0 offset won't be applied. Use nullish checks instead.
🛠️ Suggested fix
- const objs =
- nextTimeOffset && !partInstancesInfo.next // apply the lookahead offset to the first future if an offset is set.
+ const objs =
+ nextTimeOffset !== undefined && nextTimeOffset !== null && !partInstancesInfo.next // apply the lookahead offset to the first future if an offset is set.
? findLookaheadObjectsForPart(
context,
currentPartId,
@@
- if (nextTimeOffset) {
+ if (nextTimeOffset !== undefined && nextTimeOffset !== null) {
return {
objs: findLookaheadObjectsForPart(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (partInfo.pieces.length > 0 && isPartPlayable(partInfo.part)) { | |
| const objs = findLookaheadObjectsForPart( | |
| context, | |
| currentPartInstanceId, | |
| layer, | |
| previousPart, | |
| partInfo, | |
| null | |
| ) | |
| const objs = | |
| nextTimeOffset && !partInstancesInfo.next // apply the lookahead offset to the first future if an offset is set. | |
| ? findLookaheadObjectsForPart( | |
| context, | |
| currentPartId, | |
| layer, | |
| previousPart, | |
| partInfo, | |
| null, | |
| nextTimeOffset | |
| ) | |
| : findLookaheadObjectsForPart(context, currentPartId, layer, previousPart, partInfo, null) | |
| res.future.push(...objs) | |
| if (partInfo.pieces.length > 0 && isPartPlayable(partInfo.part)) { | |
| const objs = | |
| nextTimeOffset !== undefined && nextTimeOffset !== null && !partInstancesInfo.next // apply the lookahead offset to the first future if an offset is set. | |
| ? findLookaheadObjectsForPart( | |
| context, | |
| currentPartId, | |
| layer, | |
| previousPart, | |
| partInfo, | |
| null, | |
| nextTimeOffset | |
| ) | |
| : findLookaheadObjectsForPart(context, currentPartId, layer, previousPart, partInfo, null) | |
| res.future.push(...objs) |
🤖 Prompt for AI Agents
In `@packages/job-worker/src/playout/lookahead/findForLayer.ts` around lines 86 -
99, The code currently uses a truthy check on nextTimeOffset which ignores 0;
change the condition in the block that calls findLookaheadObjectsForPart to
explicitly test for null/undefined (e.g. nextTimeOffset != null) so a zero
offset is honored: use (nextTimeOffset != null && !partInstancesInfo.next) to
decide when to call findLookaheadObjectsForPart with nextTimeOffset, otherwise
call the variant without the offset; update the conditional that builds objs
(and keep the same arguments/currentPartId/layer/previousPart/partInfo) so zero
offsets are applied correctly.
| const partInfo: PartAndPieces = { | ||
| part: partInstanceInfo.part.part, | ||
| usesInTransition: partInstanceInfo.calculatedTimings?.inTransitionStart ? true : false, | ||
| pieces: sortPieceInstancesByStart(partInstanceInfo.allPieces, partInstanceInfo.nowInPart), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/job-worker/src/playout/lookahead/findForLayer.ts | head -130 | tail -25Repository: Sofie-Automation/sofie-core
Length of output: 1044
🏁 Script executed:
rg "inTransitionStart" packages/job-worker/src/playout/lookahead/ -A 2 -B 2Repository: Sofie-Automation/sofie-core
Length of output: 2319
🏁 Script executed:
rg "interface PartAndPieces|type PartAndPieces" packages/job-worker/src/ -A 10Repository: Sofie-Automation/sofie-core
Length of output: 1016
🏁 Script executed:
rg "calculatedTimings" packages/job-worker/src/ -l | head -5Repository: Sofie-Automation/sofie-core
Length of output: 330
🏁 Script executed:
rg "PartInstanceAndPieceInstances|calculatedTimings" packages/job-worker/src/ -B 2 -A 2 | head -80Repository: Sofie-Automation/sofie-core
Length of output: 6508
🏁 Script executed:
rg "inTransitionStart.*:" packages/job-worker/src/ -B 3 -A 1 | head -100Repository: Sofie-Automation/sofie-core
Length of output: 5141
🏁 Script executed:
rg "type PartCalculatedTimings|interface PartCalculatedTimings" packages/ -A 10Repository: Sofie-Automation/sofie-core
Length of output: 1072
🏁 Script executed:
rg "usesInTransition" packages/job-worker/src/playout/lookahead/ -B 2 -A 2Repository: Sofie-Automation/sofie-core
Length of output: 10179
Fix truthy check for inTransitionStart to handle zero correctly.
When inTransitionStart is 0, the current truthy check incorrectly sets usesInTransition to false, suppressing transition lookahead for transitions that start at time zero. The value 0 is a valid offset indicating the transition begins at the start of the part group.
🔧 Suggested fix
- usesInTransition: partInstanceInfo.calculatedTimings?.inTransitionStart ? true : false,
+ usesInTransition: partInstanceInfo.calculatedTimings?.inTransitionStart != null,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const partInfo: PartAndPieces = { | |
| part: partInstanceInfo.part.part, | |
| usesInTransition: partInstanceInfo.calculatedTimings?.inTransitionStart ? true : false, | |
| pieces: sortPieceInstancesByStart(partInstanceInfo.allPieces, partInstanceInfo.nowInPart), | |
| const partInfo: PartAndPieces = { | |
| part: partInstanceInfo.part.part, | |
| usesInTransition: partInstanceInfo.calculatedTimings?.inTransitionStart != null, | |
| pieces: sortPieceInstancesByStart(partInstanceInfo.allPieces, partInstanceInfo.nowInPart), |
🤖 Prompt for AI Agents
In `@packages/job-worker/src/playout/lookahead/findForLayer.ts` around lines 116 -
119, The usesInTransition boolean is computed with a truthy check that treats 0
as false; update the logic in the PartAndPieces construction (partInfo) to
explicitly detect a defined numeric inTransitionStart (e.g. check
partInstanceInfo.calculatedTimings?.inTransitionStart !== undefined &&
partInstanceInfo.calculatedTimings?.inTransitionStart !== null or typeof ===
'number') so that 0 is accepted as a valid start and usesInTransition is true
when an offset exists.
| allObjs.map((obj) => { | ||
| const piece = partInfo.pieces.find((piece) => unprotectString(piece._id) === obj.pieceInstanceId) | ||
| if (!piece) return |
There was a problem hiding this comment.
Temporary pieces can be dropped during piece lookup.
obj.pieceInstanceId can be derived from getBestPieceInstanceId (eg. startPartId) for temporary pieces, so matching by unprotectString(piece._id) can fail and silently skip objects. Consider matching via getBestPieceInstanceId(piece) instead.
🔧 Suggested fix
- allObjs.map((obj) => {
- const piece = partInfo.pieces.find((piece) => unprotectString(piece._id) === obj.pieceInstanceId)
+ allObjs.map((obj) => {
+ const piece = partInfo.pieces.find((piece) => getBestPieceInstanceId(piece) === obj.pieceInstanceId)
if (!piece) return🤖 Prompt for AI Agents
In `@packages/job-worker/src/playout/lookahead/findObjects.ts` around lines 146 -
148, The current lookup in allObjs.map uses unprotectString(piece._id) ===
obj.pieceInstanceId which can miss temporary pieces whose instance id is derived
(e.g., via startPartId); update the lookup inside allObjs.map to compare the
object's pieceInstanceId against getBestPieceInstanceId(piece) instead (i.e.,
use partInfo.pieces.find(piece => getBestPieceInstanceId(piece) ===
obj.pieceInstanceId)), keeping the rest of the mapping logic the same so
temporary pieces are not silently skipped; reference functions/values:
allObjs.map, partInfo.pieces, unprotectString, obj.pieceInstanceId, and
getBestPieceInstanceId.
| // Pick the relevant piece before/at nextTimeOffset | ||
| if (beforeOrAt.length > 0) { | ||
| const best = beforeOrAt.reduce((a, b) => (a.piece.enable.start > b.piece.enable.start ? a : b)) | ||
| result.push(best) | ||
| } |
There was a problem hiding this comment.
Potential type safety issue in piece comparison.
Line 199 compares a.piece.enable.start > b.piece.enable.start directly, but enable.start may not be a number (it could be a string expression or undefined). While beforeOrAt only contains pieces where getStartValueFromEnable returned a defined value (line 189-192), the comparison still uses the raw enable.start property rather than the extracted numeric value.
This could produce incorrect results for pieces with while-based enables or "now" starts.
🐛 Proposed fix
// Pick the relevant piece before/at nextTimeOffset
if (beforeOrAt.length > 0) {
- const best = beforeOrAt.reduce((a, b) => (a.piece.enable.start > b.piece.enable.start ? a : b))
+ const best = beforeOrAt.reduce((a, b) => {
+ const aStart = getStartValueFromEnable(a.piece.enable) ?? 0
+ const bStart = getStartValueFromEnable(b.piece.enable) ?? 0
+ return aStart > bStart ? a : b
+ })
result.push(best)
}Note: This requires making getStartValueFromEnable accessible within this scope (it's currently a module-level function, so this should work).
🤖 Prompt for AI Agents
In `@packages/job-worker/src/playout/lookahead/lookaheadOffset.ts` around lines
197 - 201, The reduce is comparing raw a.piece.enable.start instead of the
numeric start computed by getStartValueFromEnable, which can be
string/undefined; update the logic that builds or filters beforeOrAt so each
entry carries the numeric start value (call getStartValueFromEnable for each
piece) and then change the reduce in the block that picks the best (the reduce
on beforeOrAt) to compare those numeric start values (e.g. compare a.startValue
> b.startValue) rather than a.piece.enable.start; ensure getStartValueFromEnable
is referenced (it's module-level) and used consistently when constructing
beforeOrAt so the comparison is type-safe and handles "now"/while-based enables.
| "@mos-connection/model": "^5.0.0-alpha.0", | ||
| "kairos-lib": "^0.2.3", | ||
| "timeline-state-resolver-types": "10.0.0-nightly-release53-20251217-143607-df590aa96.0", | ||
| "timeline-state-resolver-types": "10.0.0-nightly-release53-20260123-151128-04b075e87.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for version consistency across package.json files
rg -n --type=json '"timeline-state-resolver-types"' --glob 'package.json'
# Confirm lockfile contains the updated version
rg -n 'timeline-state-resolver-types@' yarn.lockRepository: Sofie-Automation/sofie-core
Length of output: 197
🏁 Script executed:
#!/bin/bash
# Check if yarn.lock exists
ls -lh yarn.lock 2>&1
# Search for timeline-state-resolver-types in yarn.lock with broader pattern
rg -i 'timeline-state-resolver' yarn.lock -A 5 -B 2
# List all package.json files in the repo
find . -name 'package.json' -type f | head -20Repository: Sofie-Automation/sofie-core
Length of output: 745
🏁 Script executed:
#!/bin/bash
# Check all package.json files for timeline-state-resolver-types
rg -l '"timeline-state-resolver-types"' --glob 'package.json'
# Search yarn.lock more broadly and show surrounding context
grep -C 10 'timeline-state' yarn.lock || echo "No match found in yarn.lock"
# Show yarn.lock size and structure sample
echo "=== yarn.lock structure (first 50 lines) ==="
head -50 yarn.lockRepository: Sofie-Automation/sofie-core
Length of output: 1938
Update yarn.lock to include the nightly build dependency.
The timeline-state-resolver-types nightly build is pinned in package.json but has no corresponding entry in yarn.lock. Run yarn install to lock this dependency and ensure consistent installs across environments.
🤖 Prompt for AI Agents
In `@packages/shared-lib/package.json` at line 43, package.json now pins
"timeline-state-resolver-types" to a nightly version but yarn.lock is missing
that entry; run yarn install in the repository root to update yarn.lock so the
nightly dependency is resolved and then commit the updated yarn.lock; verify the
new entry for timeline-state-resolver-types appears in yarn.lock and that CI
passes.
5766554 to
028e64c
Compare
|
Hi, from the UI side this seems to work as expected, and a sought after feature for our users 😊 Unfortunately I don’t have access to a payout for testing, so I’m unable to test it completely. I have handed it over to another team member to test it out, bu he won’t be able to get to it until after the Olympics have started. |
…bjects with no explicit start
Co-authored-by: Julian Waller <Julusian@users.noreply.github.com>
…e is selected to be set as next.
…e expected duration
028e64c to
a859b22
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/job-worker/src/playout/lookahead/findObjects.ts (1)
146-174:⚠️ Potential issue | 🟡 MinorUse
.forEach()instead of.map()— return values are discarded.
allObjs.map(...)creates an array that is never used. This should be.forEach()to express intent and satisfy the linter.Proposed fix
- allObjs.map((obj) => { + allObjs.forEach((obj) => {
🤖 Fix all issues with AI agents
In `@meteor/server/api/userActions.ts`:
- Around line 134-137: The parameter-validation callback in userActions.ts
currently only checks rundownPlaylistId and nextPartOrInstanceId but skips
timeOffset and isInstance; update that same validation callback to assert
timeOffset and isInstance too (use Match.OneOf(Number, null) or
Match.Optional(Number) for timeOffset if it can be null/optional, and
check(isInstance, Boolean)) so all four parameters (rundownPlaylistId,
nextPartOrInstanceId, timeOffset, isInstance) are validated consistent with
other methods like take (which uses Match.OneOf for fromPartInstanceId).
In `@packages/job-worker/src/playout/lookahead/findForLayer.ts`:
- Around line 121-133: The branch in generatePartInstanceLookaheads uses if
(nextTimeOffset) which treats 0 as falsy and duplicates both branches; update
the code to collapse the branches and always call findLookaheadObjectsForPart
with nextTimeOffset normalized to the expected optional type (e.g. pass
nextTimeOffset ?? undefined) so null becomes undefined and 0 is preserved,
eliminating the truthy-vs-nullish bug and removing duplicated return logic.
In `@packages/job-worker/src/playout/setNext.ts`:
- Around line 595-609: The DBPartInstance branch is missing the
consumesQueuedSegmentId calculation, so when nextPartOrInstance is a
DBPartInstance you must determine whether its segment matches the queued segment
(using doesPartConsumeQueuedSegmentId or equivalent logic) and pass that boolean
into setNextPartAndCheckForPendingMoveNextPart (just like the DBPart path passes
consumesQueuedSegmentId into setNextPart); locate the DBPartInstance handling
around the nextPartOrInstance code and add a consumesQueuedSegmentId =
doesPartConsumeQueuedSegmentId(playoutModel, nextPartOrInstance) (or compute
based on nextPartOrInstance.part.segmentId) and forward it into
setNextPartAndCheckForPendingMoveNextPart so queuedSegmentId is cleared
correctly.
In `@packages/job-worker/src/playout/setNextJobs.ts`:
- Line 56: The assignment path that sets nextPartOrInstance = nextPartInstance
skips the isPartPlayable validation, so update setNextJobs to run the same
isPartPlayable check for nextPartInstance (use the underlying part referenced by
nextPartInstance) before accepting it as the nextPartOrInstance; if
isPartPlayable returns false, treat it the same as the unplayable-nextPartId
branch (skip or fallback) so unplayable parts are never selected. Ensure you
reference and use the existing isPartPlayable helper and handle nextPartInstance
the same way you handle the nextPartId resolution (same logic block used around
nextPartOrInstance, nextPartInstance, nextPartId).
- Around line 38-78: The handler currently silently no-ops when neither
data.nextPartInstanceId nor data.nextPartId is provided and also skips
playability validation when using a nextPartInstance; update the logic in
setNextJobs (around the nextPartInstanceId/nextPartId resolution and before
calling setNextPartFromPart) to: (1) add an else branch that throws a UserError
(use UserError.create with an appropriate UserErrorMessage and HTTP code) when
both identifiers are missing so callers don’t silently do nothing, and (2) when
using nextPartInstance (nextPartInstance.part), run the same isPartPlayable
check you run for playoutModel.findPart and throw
UserError.create(UserErrorMessage.PartNotPlayable, ...) if the embedded part is
not playable before assigning nextPartOrInstance or calling setNextPartFromPart.
In `@packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx`:
- Around line 305-325: Rename the misleading variable isSelectedTimeWithinBounds
in getIsPlayFromHereDisabled to something like isSelectedTimeOutOfBounds (or
isSelectedTimeExceedsDuration) since it's computed as (duration < offset);
update its declaration and the usage in the return expression inside
getIsPlayFromHereDisabled so the boolean name matches the semantics (keep the
logic the same), referencing getTimePosition(), getPartFromContext()?.instance,
and playlist.currentPartInfo?.partInstanceId to locate the surrounding code.
- Around line 315-324: Update the disabled logic for the menu items that call
getIsPlayFromHereDisabled() in SegmentContextMenu.tsx (the "Set part from ... as
Next" and "Play part from ..." menu entries) to also respect the playlist
activation check used elsewhere: add a `|| !canSetAsNext` condition to their
disabled props so they are disabled when the playlist is inactive (consistent
with other items that use `!!part.instance.orphaned || !canSetAsNext`); no
change to onSetNext is needed here, just include `|| !canSetAsNext` alongside
the existing getIsPlayFromHereDisabled() check.
🧹 Nitpick comments (8)
packages/job-worker/src/playout/setNextJobs.ts (1)
49-53: Condition logic could benefit from a clearer comment.The comment says "We can't reuse the currently playing instance," but the condition also falls back to using the Part ID when there is no
nextPartInfoor nocurrentPartInfo. Those first two sub-conditions aren't explained and aren't obvious — it would help future readers to understand why the absence of next/current info requires falling back to the Part rather than reusing the instance.packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx (1)
74-78:isPartNextandisPartOrphanedguards are redundant with thepart &&check.Both
isPartNextandisPartOrphanedareundefinedonly whenpartis falsy (lines 74–78). SoisPartNext !== undefined && isPartOrphaned !== undefinedat lines 132–133 adds no filtering beyondpart &&at line 131. If these variables are intended to carry semantic meaning for rendering decisions (beyond null-guarding), they aren't used that way — consider removing the redundant checks or using the boolean values directly.Also applies to: 131-135
packages/job-worker/src/playout/setNext.ts (1)
576-578: JSDoc@paramdescription is stale.The description still says "The Part to set as next" but the parameter now also accepts a
DBPartInstance.Suggested fix
- * `@param` nextPartOrInstance The Part to set as next + * `@param` nextPartOrInstance The Part or PartInstance to set as nextpackages/job-worker/src/playout/lookahead/__tests__/lookaheadOffset/constants.ts (1)
43-43: Typo in function name:generateFakeObectsString→generateFakeObjectsString.The function name is missing a "j" —
Obectsinstead ofObjects. Since this is exported and referenced from test files, a rename now avoids confusion later.packages/job-worker/src/playout/lookahead/__tests__/findForLayer/timing.test.ts (1)
11-13:mockReset()clears the default implementation set on line 9.
mockReset()removes themockImplementation(() => [])established earlier. This works here because each test supplies its ownmockReturnValueOncevalues, but it means any test that doesn't explicitly set a return will getundefinedrather than[]. Consider usingmockClear()if you want the default to persist, or document that each test must set its own return values.packages/job-worker/src/playout/lookahead/__tests__/lookaheadOffset/lookaheadOffset.test.ts (1)
27-32:jest.resetAllMocks()clears module-level mock implementations.After
resetAllMocks(), thefindLargestLookaheadDistanceMock(line 20) andgetOrderedPartsAfterPlayheadMock(line 21) lose their default implementations. Tests that don't re-mockfindLargestLookaheadDistancebefore callinggetLookeaheadObjectswill getundefinedfrom it, causinggetOrderedPartsAfterPlayheadto be called withundefinedas thepartCountargument. This works here becausegetOrderedPartsAfterPlayheadis itself mocked, but it could hide bugs if the interaction ever matters. Considerjest.clearAllMocks()or re-establishing defaults after reset.packages/job-worker/src/playout/lookahead/__tests__/findForLayer/constants.ts (1)
7-29: MissingcalculatedTimingsandnowInParton some test fixtures.The
PartInstanceAndPieceInstancesinterface requirescalculatedTimingsandnowInPart, but none of the fixtures providecalculatedTimings, andnextTimed/nextFutureomitnowInPart. Theas anycast hides this. Currently safe because callers use optional chaining (calculatedTimings?.inTransitionStart), but if any consuming test or production code ever accesses these without?., it will throw at runtime.Consider adding at least stub values (e.g.,
calculatedTimings: {},nowInPart: 0) to reduce brittleness.packages/job-worker/src/playout/lookahead/findForLayer.ts (1)
108-108: Missing blank line before function declaration.Minor nit: add a blank line before
function generatePartInstanceLookaheadsfor readability, consistent with the rest of the file's spacing conventions.
| () => { | ||
| check(rundownPlaylistId, String) | ||
| check(nextPartId, String) | ||
| check(nextPartOrInstanceId, String) | ||
| }, |
There was a problem hiding this comment.
Missing validation for timeOffset and isInstance parameters.
The validation callback only checks rundownPlaylistId and nextPartOrInstanceId, but omits timeOffset and isInstance. Every other method in this class validates all incoming parameters (e.g., take validates fromPartInstanceId with Match.OneOf). Without these checks, malformed inputs bypass Meteor's type enforcement.
Proposed fix
() => {
check(rundownPlaylistId, String)
check(nextPartOrInstanceId, String)
+ check(timeOffset, Match.OneOf(Number, null))
+ check(isInstance, Match.OneOf(Boolean, null))
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| () => { | |
| check(rundownPlaylistId, String) | |
| check(nextPartId, String) | |
| check(nextPartOrInstanceId, String) | |
| }, | |
| () => { | |
| check(rundownPlaylistId, String) | |
| check(nextPartOrInstanceId, String) | |
| check(timeOffset, Match.OneOf(Number, null)) | |
| check(isInstance, Match.OneOf(Boolean, null)) | |
| }, |
🤖 Prompt for AI Agents
In `@meteor/server/api/userActions.ts` around lines 134 - 137, The
parameter-validation callback in userActions.ts currently only checks
rundownPlaylistId and nextPartOrInstanceId but skips timeOffset and isInstance;
update that same validation callback to assert timeOffset and isInstance too
(use Match.OneOf(Number, null) or Match.Optional(Number) for timeOffset if it
can be null/optional, and check(isInstance, Boolean)) so all four parameters
(rundownPlaylistId, nextPartOrInstanceId, timeOffset, isInstance) are validated
consistent with other methods like take (which uses Match.OneOf for
fromPartInstanceId).
| if (nextTimeOffset) { | ||
| return { | ||
| objs: findLookaheadObjectsForPart( | ||
| context, | ||
| currentPartInstanceId, | ||
| layer, | ||
| previousPart, | ||
| partInfo, | ||
| partInstanceInfo.part._id, | ||
| nextTimeOffset | ||
| ), | ||
| partInfo, | ||
| } |
There was a problem hiding this comment.
Same truthy-vs-nullish bug for nextTimeOffset in generatePartInstanceLookaheads.
if (nextTimeOffset) on Line 121 has the same 0-is-falsy issue. Additionally, the two branches are identical except for the nextTimeOffset argument — this can be simplified since findLookaheadObjectsForPart already accepts nextTimeOffset as optional.
Proposed fix — collapse the branches
- if (nextTimeOffset) {
- return {
- objs: findLookaheadObjectsForPart(
- context,
- currentPartInstanceId,
- layer,
- previousPart,
- partInfo,
- partInstanceInfo.part._id,
- nextTimeOffset
- ),
- partInfo,
- }
- } else {
- return {
- objs: findLookaheadObjectsForPart(
- context,
- currentPartInstanceId,
- layer,
- previousPart,
- partInfo,
- partInstanceInfo.part._id
- ),
- partInfo,
- }
- }
+ return {
+ objs: findLookaheadObjectsForPart(
+ context,
+ currentPartInstanceId,
+ layer,
+ previousPart,
+ partInfo,
+ partInstanceInfo.part._id,
+ nextTimeOffset ?? undefined
+ ),
+ partInfo,
+ }This eliminates the truthy bug and the code duplication in one change. The ?? undefined converts null to undefined to match the parameter type.
🤖 Prompt for AI Agents
In `@packages/job-worker/src/playout/lookahead/findForLayer.ts` around lines 121 -
133, The branch in generatePartInstanceLookaheads uses if (nextTimeOffset) which
treats 0 as falsy and duplicates both branches; update the code to collapse the
branches and always call findLookaheadObjectsForPart with nextTimeOffset
normalized to the expected optional type (e.g. pass nextTimeOffset ?? undefined)
so null becomes undefined and 0 is preserved, eliminating the truthy-vs-nullish
bug and removing duplicated return logic.
| let consumesQueuedSegmentId: boolean | undefined | ||
|
|
||
| await setNextPart(context, playoutModel, { part: nextPart, consumesQueuedSegmentId }, setManually, nextTimeOffset) | ||
| if (!('part' in nextPartOrInstance)) { | ||
| consumesQueuedSegmentId = doesPartConsumeQueuedSegmentId(playoutModel, nextPartOrInstance) | ||
|
|
||
| await setNextPart( | ||
| context, | ||
| playoutModel, | ||
| { | ||
| part: nextPartOrInstance, | ||
| consumesQueuedSegmentId, | ||
| }, | ||
| setManually, | ||
| nextTimeOffset | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the setNext.ts file
find . -name "setNext.ts" -type fRepository: Sofie-Automation/sofie-core
Length of output: 115
🏁 Script executed:
# Get the full context of lines 595-625 to see both code paths
wc -l packages/job-worker/src/playout/setNext.tsRepository: Sofie-Automation/sofie-core
Length of output: 117
🏁 Script executed:
# Read the relevant section of the file
sed -n '590,630p' packages/job-worker/src/playout/setNext.ts | cat -nRepository: Sofie-Automation/sofie-core
Length of output: 1589
🏁 Script executed:
# Search for setNextPartAndCheckForPendingMoveNextPart to understand its implementation
rg -n "setNextPartAndCheckForPendingMoveNextPart" packages/job-worker/src/playout/setNext.ts -A 20 -B 2Repository: Sofie-Automation/sofie-core
Length of output: 2532
🏁 Script executed:
# Look for the line 131 mentioned in the review comment
sed -n '125,140p' packages/job-worker/src/playout/setNext.ts | cat -nRepository: Sofie-Automation/sofie-core
Length of output: 909
🏁 Script executed:
# Search for doesPartConsumeQueuedSegmentId function
rg -n "doesPartConsumeQueuedSegmentId" packages/job-worker/src/playout/setNext.ts -A 5 -B 2Repository: Sofie-Automation/sofie-core
Length of output: 694
🏁 Script executed:
# Find the function definition that contains lines 595-625
sed -n '570,630p' packages/job-worker/src/playout/setNext.ts | cat -nRepository: Sofie-Automation/sofie-core
Length of output: 2379
🏁 Script executed:
# Check the setNextPart function signature to see how it uses consumesQueuedSegmentId
rg -n "async function setNextPart|function setNextPart" packages/job-worker/src/playout/setNext.ts -A 15Repository: Sofie-Automation/sofie-core
Length of output: 2112
🏁 Script executed:
# Look for any comments or context around the DBPartInstance path
sed -n '600,630p' packages/job-worker/src/playout/setNext.ts | cat -nRepository: Sofie-Automation/sofie-core
Length of output: 1084
🏁 Script executed:
# Get the full implementation of doesPartConsumeQueuedSegmentId
sed -n '628,660p' packages/job-worker/src/playout/setNext.ts | cat -nRepository: Sofie-Automation/sofie-core
Length of output: 649
🏁 Script executed:
# Check how consumesQueuedSegmentId is used in the rest of the logic
rg -n "consumesQueuedSegmentId" packages/job-worker/src/playout/setNext.ts -B 2 -A 2Repository: Sofie-Automation/sofie-core
Length of output: 1582
🏁 Script executed:
# Search for any related tests or comments that explain the intention
rg -n "queued" packages/job-worker/src/playout/setNext.ts -i -B 1 -A 1Repository: Sofie-Automation/sofie-core
Length of output: 2756
Missing consumesQueuedSegmentId calculation for DBPartInstance path leads to queued segment not being cleared.
When setting a DBPart as next, doesPartConsumeQueuedSegmentId properly determines whether the queued segment should be cleared (lines 598). However, when setting a DBPartInstance as next (lines 610–626), this calculation is omitted, causing consumesQueuedSegmentId to default to false in setNextPartAndCheckForPendingMoveNextPart (line 131). As a result, if a user sets a part instance from the queued segment as next, the queued segment won't be cleared—unlike the behavior for DBPart.
This should be fixed by calculating whether the part instance's segment matches the queued segment and passing the correct consumesQueuedSegmentId value, consistent with the DBPart path logic.
🤖 Prompt for AI Agents
In `@packages/job-worker/src/playout/setNext.ts` around lines 595 - 609, The
DBPartInstance branch is missing the consumesQueuedSegmentId calculation, so
when nextPartOrInstance is a DBPartInstance you must determine whether its
segment matches the queued segment (using doesPartConsumeQueuedSegmentId or
equivalent logic) and pass that boolean into
setNextPartAndCheckForPendingMoveNextPart (just like the DBPart path passes
consumesQueuedSegmentId into setNextPart); locate the DBPartInstance handling
around the nextPartOrInstance code and add a consumesQueuedSegmentId =
doesPartConsumeQueuedSegmentId(playoutModel, nextPartOrInstance) (or compute
based on nextPartOrInstance.part.segmentId) and forward it into
setNextPartAndCheckForPendingMoveNextPart so queuedSegmentId is cleared
correctly.
| let nextPartOrInstance: ReadonlyDeep<DBPart> | DBPartInstance | undefined | ||
| let nextPartId: PartId | undefined | ||
|
|
||
| if (data.nextPartInstanceId) { | ||
| // Fetch the part instance | ||
| const nextPartInstance = await context.directCollections.PartInstances.findOne({ | ||
| _id: data.nextPartInstanceId, | ||
| }) | ||
| if (!nextPartInstance) throw UserError.create(UserErrorMessage.PartNotFound, undefined, 404) | ||
|
|
||
| // Determine if we need the part itself or can use the instance (We can't reuse the currently playing instance) | ||
| if ( | ||
| !playlist.nextPartInfo?.partInstanceId || | ||
| !playlist.currentPartInfo?.partInstanceId || | ||
| playlist.currentPartInfo?.partInstanceId === data.nextPartInstanceId | ||
| ) { | ||
| nextPartId = nextPartInstance.part._id | ||
| } else { | ||
| nextPartOrInstance = nextPartInstance | ||
| } | ||
| } else if (data.nextPartId) { | ||
| nextPartId = data.nextPartId | ||
| } | ||
|
|
||
| await setNextPartFromPart(context, playoutModel, nextPart, data.setManually ?? false, data.nextTimeOffset) | ||
| // If we have a nextPartId, resolve the actual part | ||
| if (nextPartId) { | ||
| const nextPart = playoutModel.findPart(nextPartId) | ||
| if (!nextPart) throw UserError.create(UserErrorMessage.PartNotFound, undefined, 404) | ||
| if (!isPartPlayable(nextPart)) throw UserError.create(UserErrorMessage.PartNotPlayable, undefined, 412) | ||
| nextPartOrInstance = nextPart | ||
| } | ||
|
|
||
| if (nextPartOrInstance) { | ||
| await setNextPartFromPart( | ||
| context, | ||
| playoutModel, | ||
| nextPartOrInstance, | ||
| data.setManually ?? false, | ||
| data.nextTimeOffset | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the SetNextPartProps interface definition
rg -n "interface SetNextPartProps" -A 10 --type=tsRepository: Sofie-Automation/sofie-core
Length of output: 915
🏁 Script executed:
#!/bin/bash
# Search for where SetNextPartProps is used or where this function is called
rg -n "setNextPartProps\|SetNextPartProps\|data\.nextPartInstanceId\|data\.nextPartId" --type=ts -B 2 -A 2 | head -120Repository: Sofie-Automation/sofie-core
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Examine the complete handleSetNextPart function to understand control flow
rg -n "handleSetNextPart|setNextPartFromPart" --type=ts -B 5 -A 15 | head -150Repository: Sofie-Automation/sofie-core
Length of output: 12973
🏁 Script executed:
#!/bin/bash
# Look at the complete setNextJobs.ts file to see full context
wc -l packages/job-worker/src/playout/setNextJobs.tsRepository: Sofie-Automation/sofie-core
Length of output: 121
Throw an error when neither nextPartInstanceId nor nextPartId is provided.
If both data.nextPartInstanceId and data.nextPartId are falsy, nextPartOrInstance remains undefined, the guard at line 70 is skipped, and the function falls through to call only updateTimeline without actually setting a next part or raising an error. Since SetNextPartProps defines both fields as optional, callers may inadvertently trigger a silent no-op. Add an else branch to throw UserError when neither identifier is supplied.
Additionally, when nextPartInstanceId is provided and the code uses the instance directly (line 56), the isPartPlayable check is skipped. This check is only performed on the nextPartId path (line 66). Consider validating playability for the instance's embedded part as well.
🤖 Prompt for AI Agents
In `@packages/job-worker/src/playout/setNextJobs.ts` around lines 38 - 78, The
handler currently silently no-ops when neither data.nextPartInstanceId nor
data.nextPartId is provided and also skips playability validation when using a
nextPartInstance; update the logic in setNextJobs (around the
nextPartInstanceId/nextPartId resolution and before calling setNextPartFromPart)
to: (1) add an else branch that throws a UserError (use UserError.create with an
appropriate UserErrorMessage and HTTP code) when both identifiers are missing so
callers don’t silently do nothing, and (2) when using nextPartInstance
(nextPartInstance.part), run the same isPartPlayable check you run for
playoutModel.findPart and throw
UserError.create(UserErrorMessage.PartNotPlayable, ...) if the embedded part is
not playable before assigning nextPartOrInstance or calling setNextPartFromPart.
| ) { | ||
| nextPartId = nextPartInstance.part._id | ||
| } else { | ||
| nextPartOrInstance = nextPartInstance |
There was a problem hiding this comment.
Missing isPartPlayable check when reusing a DBPartInstance directly.
When the code takes the nextPartOrInstance = nextPartInstance path (line 56), no playability validation occurs. The isPartPlayable check at line 66 only applies when resolving via nextPartId. If the underlying part became unplayable after the instance was created, this would allow nexting to an unplayable part.
Proposed fix
} else {
- nextPartOrInstance = nextPartInstance
+ if (!isPartPlayable(nextPartInstance.part))
+ throw UserError.create(UserErrorMessage.PartNotPlayable, undefined, 412)
+ nextPartOrInstance = nextPartInstance
}Also applies to: 62-68
🤖 Prompt for AI Agents
In `@packages/job-worker/src/playout/setNextJobs.ts` at line 56, The assignment
path that sets nextPartOrInstance = nextPartInstance skips the isPartPlayable
validation, so update setNextJobs to run the same isPartPlayable check for
nextPartInstance (use the underlying part referenced by nextPartInstance) before
accepting it as the nextPartOrInstance; if isPartPlayable returns false, treat
it the same as the unplayable-nextPartId branch (skip or fallback) so unplayable
parts are never selected. Ensure you reference and use the existing
isPartPlayable helper and handle nextPartInstance the same way you handle the
nextPartId resolution (same logic block used around nextPartOrInstance,
nextPartInstance, nextPartId).
| private getIsPlayFromHereDisabled(take: boolean = false): boolean { | ||
| const offset = this.getTimePosition() ?? 0 | ||
| const playlist = this.props.playlist | ||
| const partInstance = this.getPartFromContext()?.instance | ||
| const isSelectedTimeWithinBounds = | ||
| (partInstance?.part.expectedDuration ?? | ||
| partInstance?.part.displayDuration ?? | ||
| partInstance?.part.expectedDurationWithTransition ?? | ||
| 0) < offset | ||
|
|
||
| // private onSetAsNextFromHere = (part: DBPart, e) => { | ||
| // const offset = this.getTimePosition() | ||
| // this.props.onSetNext(part, e, offset || 0) | ||
| // } | ||
| if (playlist && playlist?.activationId && (!take || !!partInstance?.orphaned)) { | ||
| if (!partInstance) return true | ||
| else { | ||
| return ( | ||
| (isSelectedTimeWithinBounds && partInstance._id === playlist.currentPartInfo?.partInstanceId) || | ||
| (!!partInstance.orphaned && partInstance._id === playlist.currentPartInfo?.partInstanceId) | ||
| ) | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Misleading variable name: isSelectedTimeWithinBounds actually means the opposite.
The condition duration < offset is true when the selected time exceeds the part's duration (i.e., is out of bounds). The variable name suggests the inverse. This will confuse future maintainers even though the downstream usage at line 319 produces the correct disable behavior.
Proposed rename
- const isSelectedTimeWithinBounds =
+ const isSelectedTimeOutOfBounds =
(partInstance?.part.expectedDuration ??
partInstance?.part.displayDuration ??
partInstance?.part.expectedDurationWithTransition ??
0) < offsetAnd update line 319 accordingly (the logic remains the same since the name now matches the semantics).
🤖 Prompt for AI Agents
In `@packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx` around
lines 305 - 325, Rename the misleading variable isSelectedTimeWithinBounds in
getIsPlayFromHereDisabled to something like isSelectedTimeOutOfBounds (or
isSelectedTimeExceedsDuration) since it's computed as (duration < offset);
update its declaration and the usage in the return expression inside
getIsPlayFromHereDisabled so the boolean name matches the semantics (keep the
logic the same), referencing getTimePosition(), getPartFromContext()?.instance,
and playlist.currentPartInfo?.partInstanceId to locate the surrounding code.
| if (playlist && playlist?.activationId && (!take || !!partInstance?.orphaned)) { | ||
| if (!partInstance) return true | ||
| else { | ||
| return ( | ||
| (isSelectedTimeWithinBounds && partInstance._id === playlist.currentPartInfo?.partInstanceId) || | ||
| (!!partInstance.orphaned && partInstance._id === playlist.currentPartInfo?.partInstanceId) | ||
| ) | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the SegmentContextMenu.tsx file to understand the context
fd "SegmentContextMenu.tsx" --type fRepository: Sofie-Automation/sofie-core
Length of output: 138
🏁 Script executed:
# Check the size and examine the relevant sections
wc -l packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsxRepository: Sofie-Automation/sofie-core
Length of output: 142
🏁 Script executed:
# Read the full context of the menu items and the getIsPlayFromHereDisabled function
cat -n packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx | sed -n '130,180p'Repository: Sofie-Automation/sofie-core
Length of output: 2140
🏁 Script executed:
# Also check the function definition around line 315-324
cat -n packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx | sed -n '300,330p'Repository: Sofie-Automation/sofie-core
Length of output: 1333
🏁 Script executed:
# Now run the requested verification - check how onSetNext is wired
rg -n "onSetNext" --type=tsx --type=ts -C3 -g '!**/SegmentContextMenu*'Repository: Sofie-Automation/sofie-core
Length of output: 98
🏁 Script executed:
# Search for onSetNext usage with corrected syntax
rg -n "onSetNext" --type ts --type tsx -C3 -g '!**/SegmentContextMenu*'Repository: Sofie-Automation/sofie-core
Length of output: 98
🏁 Script executed:
# Also check the canSetAsNext variable definition in the file
rg -n "canSetAsNext" packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx -B2 -A2Repository: Sofie-Automation/sofie-core
Length of output: 1537
🏁 Script executed:
# Check the parent component that uses SegmentContextMenu to understand how onSetNext is provided
rg -n "SegmentContextMenu" --type ts --type tsx -l -g '!**/SegmentContextMenu.tsx'Repository: Sofie-Automation/sofie-core
Length of output: 98
🏁 Script executed:
# Let me try without specifying file type
rg -n "onSetNext" -C3 | head -100Repository: Sofie-Automation/sofie-core
Length of output: 6940
🏁 Script executed:
# Check what files import SegmentContextMenu
fd "SegmentContextMenu" --type fRepository: Sofie-Automation/sofie-core
Length of output: 138
🏁 Script executed:
# Let's also look at the full context around canSetAsNext definition to understand better
cat -n packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx | sed -n '70,110p'Repository: Sofie-Automation/sofie-core
Length of output: 2095
🏁 Script executed:
# And check the other menu items that use canSetAsNext to see the pattern
cat -n packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx | sed -n '190,230p'Repository: Sofie-Automation/sofie-core
Length of output: 1794
🏁 Script executed:
# Check the parent onSetNext handler in RundownView.tsx around line 755-778
cat -n packages/webui/src/client/ui/RundownView.tsx | sed -n '755,790p'Repository: Sofie-Automation/sofie-core
Length of output: 1613
🏁 Script executed:
# Check if there are other usages of canSetAsNext in the same file to understand the pattern
cat -n packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx | sed -n '147,190p'Repository: Sofie-Automation/sofie-core
Length of output: 2001
Inconsistent disable logic: items at lines 158 and 176 should include the !canSetAsNext check
The menu items "Set part from ... as Next" (line 158) and "Play part from ..." (line 176) only use getIsPlayFromHereDisabled() to determine their disabled state. However, when the playlist is inactive (!playlist?.activationId), this function returns false (enabled).
In contrast, other playlist-modifying items consistently disable when !canSetAsNext:
- Line 139 "Set part as Next":
disabled={!!part.instance.orphaned || !canSetAsNext} - Line 200 "Set as QuickLoop Start":
disabled={!!part.instance.orphaned || !canSetAsNext} - Line 217 "Set as QuickLoop End":
disabled={!!part.instance.orphaned || !canSetAsNext}
Since canSetAsNext = !!this.props.playlist?.activationId, these items remain disabled when the playlist is inactive. Lines 158 and 176 should also include || !canSetAsNext to maintain consistent behavior across all playlist-modifying menu actions. The parent handler onSetNext in RundownView.tsx does not guard against inactive playlists.
🤖 Prompt for AI Agents
In `@packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx` around
lines 315 - 324, Update the disabled logic for the menu items that call
getIsPlayFromHereDisabled() in SegmentContextMenu.tsx (the "Set part from ... as
Next" and "Play part from ..." menu entries) to also respect the playlist
activation check used elsewhere: add a `|| !canSetAsNext` condition to their
disabled props so they are disabled when the playlist is inactive (consistent
with other items that use `!!part.instance.orphaned || !canSetAsNext`); no
change to onSetNext is needed here, just include `|| !canSetAsNext` alongside
the existing getIsPlayFromHereDisabled() check.
About the Contributor
This pull request is posted on behalf of the BBC.
Type of Contribution
This is a: Feature
Current Behavior
Currently there is no way to set the next part with a time offset from the Rundown View.
New Behavior
Testing
Affected areas
Time Frame
This feature is critical for us, please review and merge it as soon as possible.
Other Information
This PR is depends on: Sofie-Automation/sofie-timeline-state-resolver#413
Status
This PR implements "set next from here" with time-offset-aware lookahead and support for selecting next by PartInstance, plus related UI, playout, lookahead refactors and many new/updated tests.